Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVE] OAuth Role Sync #13761

Merged
merged 27 commits into from
Apr 15, 2019
Merged

[IMPROVE] OAuth Role Sync #13761

merged 27 commits into from
Apr 15, 2019

Conversation

hypery2k
Copy link
Contributor

see #12992

@geekgonecrazy geekgonecrazy changed the title IMPROVE] Improved support for OAuth Provider [IMPROVE] Improved support for OAuth Provider Mar 27, 2019
@hypery2k
Copy link
Contributor Author

hypery2k commented Apr 3, 2019

The error in circle seems to not being related to my changes:

Element <div class="sidebar-item__ellipsis">...</div> is not clickable at point (184, 197). Other element would receive the click: <div class="sidebar-item__ellipsis">...</div>

Any chance to get this merged into master?

@hypery2k
Copy link
Contributor Author

@geekgonecrazy ok for you?

@geekgonecrazy geekgonecrazy changed the title [IMPROVE] Improved support for OAuth Provider [IMPROVE] OAuth Role Sync Apr 12, 2019
Copy link
Member

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please at least add the english i18n key for Accounts_OAuth_Custom_Merge_Roles don't need to bother with all the other languages in the scope of this PR

hypery2k and others added 2 commits April 13, 2019 07:16
Co-Authored-By: hypery2k <contact@martinreinhardt-online.de>
Co-Authored-By: hypery2k <contact@martinreinhardt-online.de>
@hypery2k
Copy link
Contributor Author

didn't get my circleci is failing.

@geekgonecrazy
Copy link
Member

circleci has been having a number of issues. :(

I've pushed a commit making some slight changes.

  1. to not add roles directly to the user in the map. Instead just return the roles as an array.
  2. To only loop over those that should be added or removed by filtering down the difference

geekgonecrazy
geekgonecrazy previously approved these changes Apr 13, 2019
@hypery2k
Copy link
Contributor Author

thanks

@geekgonecrazy
Copy link
Member

Just tried out with Okta. It works beautifully! :) Made one final adjustment

geekgonecrazy
geekgonecrazy previously approved these changes Apr 13, 2019
@hypery2k
Copy link
Contributor Author

great, i check it with latest keycloak and also works perfect. I will try to create another PR with some basic tests.
Would be end2end testing be needed? Could give it try in this new PR, if wanted.

@geekgonecrazy
Copy link
Member

geekgonecrazy commented Apr 13, 2019

Perfect! I think this PR is good to go then! :)

Sorry your other PR didn’t get addressed. :(

e2e testing would be great! But I think it would be incredibly difficult. Because would need an oauth provider that could consistently work in ci + provide roles.

But if you want to open a Pr to add testing of any sort to oauth.. that would be fantastic. Maybe just unit tests to ensure all of this logic holds sound?

@geekgonecrazy geekgonecrazy merged commit dd76eca into RocketChat:develop Apr 15, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read Group info from OAuth provider
3 participants